Minimal-printf: Set default configurations to false #11450

hugueskamba · 2019-09-10T13:41:05Z

Description

Mbed OS should not require floating point in its base configuration.
This provides further code size savings out of the box.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@evedon @kjbracey-arm

Release Notes

0xc0170 · 2019-09-10T13:55:33Z

@evedon is this needed to be in 5.14? Based on the changes - adding new configuration/changing current one, this would go to 5.15.

hugueskamba · 2019-09-10T14:17:59Z

This force-push only leaves the commit which set floating point support to false by default.

evedon · 2019-09-10T14:33:21Z

@evedon is this needed to be in 5.14? Based on the changes - adding new configuration/changing current one, this would go to 5.15.

We should target 5.14.0 RC2.

evedon · 2019-09-10T14:34:19Z

platform/source/minimal-printf/README.md

@@ -51,7 +51,7 @@ Minimal printf is configured by the following parameters defined in `platform/mb
 }
 ```

-By default, 64 bit integers, floating point  and FILE stream printing are enabled.
+By default, 64 bit integers and FILE stream printing are enabled.


We should remove all references to FILE stream printing since the option to disable/enable has been removed in a previous PR.
This also applies to the tables at the end of the file

I will have to re-build the example project and get new numbers for the first two rows in each table as the numbers do not include the enabled file stream support. But this should not be blocking the release as it can be update later.

evedon · 2019-09-10T14:38:47Z

TESTS/mbed_platform/minimal-printf/compliance/test_app.json

+{
+    "target_overrides": {
+        "*": {
+            "platform.minimal-printf-enable-floating-point": true


You will need to pass the option when running greentea tests
--app-config ./TESTS/minimal-printf/compliance/test_app.json

@jamesbeyond suggested we re-define the macro in the test main.cpp.
Though I am not sure if that helps because compilation of the minimal-printf source seems to happen before the macro re-definition in main.cpp.
In any case, the test passe regardless of the macro redefinition therefore this IMO shouldn't also block the release and can be looked at later.

ciarmcom · 2019-09-10T15:00:30Z

@hugueskamba, thank you for your changes.
@kjbracey-arm @evedon @ARMmbed/mbed-os-test @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

kjbracey · 2019-09-10T16:35:20Z

It seems reasonable for the default of the "minimal" to be more minimal, but this is an API change. If it doesn't land in 5.14, then I think 5.15 is too late to change it, as it will have been published with the default the other way.

0xc0170 · 2019-09-10T17:31:50Z

Set to 5.14.0-rc2. Lets make this ready by tomorrow noon

cc @adbridge

hugueskamba · 2019-09-10T18:38:34Z

This force-push ensures floating point support is enabled for the Greentea test. It also modifies the minimal-printf README to remove mentions of an optional file stream support.

hugueskamba · 2019-09-11T03:45:21Z

This force-push removes the macro re-definition as it does not help compiling minimal-printf with floating point support since the re-definition only happened later on.
The test does not fail, however, the side effect is that the minimal-printf floating point tests are not run.
The bigger question here (not just related to minimal-printf) is "how do we run tests for configuration options that are disabled by default?". There needs to be a generic way to test features that are off by default.
@jamesbeyond

Mbed OS should not require floating point in its base configuration. This provides further code size savings out of the box.

hugueskamba · 2019-09-11T04:10:24Z

This force-push updates the code size values in the minimal-printf README tables.

0xc0170 · 2019-09-11T06:09:13Z

Started CI to get test results while we review

mbed-ci · 2019-09-11T07:47:53Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

evedon · 2019-09-11T08:20:49Z

I agree that the default should be to have float support disabled.

I will create a ticket for the testing issue:

The bigger question here (not just related to minimal-printf) is "how do we run tests for configuration options that are disabled by default?". There needs to be a generic way to test features that are off by default.
@jamesbeyond

evedon

I'd like one more change to the greentea test before this is merged though.
@hugueskamba please put back test_app.json and add a README file with the command line to use to test for floats

evedon

We will make the change to test_app.json in a different PR; it can come in 5.14.1 as well.

hugueskamba · 2019-09-11T08:51:48Z

We will make the change to test_app.json in a different PR; it can come in 5.14.1 as well.

Here is the PR #11455

hugueskamba mentioned this pull request Sep 10, 2019

Move script to check for floats and make it Python 2 compatible #11429

Merged

hugueskamba force-pushed the hk-minimal-printf-default-options-false branch 2 times, most recently from f6c410c to 3177671 Compare September 10, 2019 14:16

evedon reviewed Sep 10, 2019

View reviewed changes

ciarmcom requested review from evedon, kjbracey and a team September 10, 2019 15:00

ciarmcom added the needs: review label Sep 10, 2019

0xc0170 added the release-version: 5.14.0-rc2 label Sep 10, 2019

0xc0170 added needs: work risk: A and removed needs: review labels Sep 10, 2019

hugueskamba force-pushed the hk-minimal-printf-default-options-false branch from 3177671 to e51e56e Compare September 10, 2019 18:34

hugueskamba force-pushed the hk-minimal-printf-default-options-false branch from e51e56e to 2195c4b Compare September 11, 2019 03:39

minimal-printf: Disable floating point support by default

03c484a

Mbed OS should not require floating point in its base configuration. This provides further code size savings out of the box.

hugueskamba force-pushed the hk-minimal-printf-default-options-false branch from 2195c4b to 03c484a Compare September 11, 2019 04:09

0xc0170 removed the needs: work label Sep 11, 2019

0xc0170 added the needs: review label Sep 11, 2019

0xc0170 approved these changes Sep 11, 2019

View reviewed changes

kjbracey approved these changes Sep 11, 2019

View reviewed changes

evedon requested changes Sep 11, 2019

View reviewed changes

evedon approved these changes Sep 11, 2019

View reviewed changes

hugueskamba mentioned this pull request Sep 11, 2019

minimal-printf: Add capability to run floating point tests manually #11455

Merged

0xc0170 added ready for merge and removed needs: review labels Sep 11, 2019

0xc0170 merged commit 91515fe into ARMmbed:master Sep 11, 2019

0xc0170 removed the ready for merge label Sep 11, 2019

hugueskamba deleted the hk-minimal-printf-default-options-false branch November 18, 2019 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimal-printf: Set default configurations to false #11450

Minimal-printf: Set default configurations to false #11450

hugueskamba commented Sep 10, 2019 •

edited

Loading

0xc0170 commented Sep 10, 2019

hugueskamba commented Sep 10, 2019

evedon commented Sep 10, 2019

evedon Sep 10, 2019 •

edited

Loading

hugueskamba Sep 10, 2019 •

edited

Loading

evedon Sep 10, 2019

hugueskamba Sep 10, 2019 •

edited

Loading

ciarmcom commented Sep 10, 2019

kjbracey commented Sep 10, 2019

0xc0170 commented Sep 10, 2019

hugueskamba commented Sep 10, 2019

hugueskamba commented Sep 11, 2019 •

edited

Loading

hugueskamba commented Sep 11, 2019

0xc0170 commented Sep 11, 2019

mbed-ci commented Sep 11, 2019

evedon commented Sep 11, 2019

evedon left a comment

evedon left a comment

hugueskamba commented Sep 11, 2019

Minimal-printf: Set default configurations to false #11450

Minimal-printf: Set default configurations to false #11450

Conversation

hugueskamba commented Sep 10, 2019 • edited Loading

Description

Pull request type

Reviewers

Release Notes

0xc0170 commented Sep 10, 2019

hugueskamba commented Sep 10, 2019

evedon commented Sep 10, 2019

evedon Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

hugueskamba Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

evedon Sep 10, 2019

Choose a reason for hiding this comment

hugueskamba Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

ciarmcom commented Sep 10, 2019

kjbracey commented Sep 10, 2019

0xc0170 commented Sep 10, 2019

hugueskamba commented Sep 10, 2019

hugueskamba commented Sep 11, 2019 • edited Loading

hugueskamba commented Sep 11, 2019

0xc0170 commented Sep 11, 2019

mbed-ci commented Sep 11, 2019

Test run: SUCCESS

evedon commented Sep 11, 2019

evedon left a comment

Choose a reason for hiding this comment

evedon left a comment

Choose a reason for hiding this comment

hugueskamba commented Sep 11, 2019

hugueskamba commented Sep 10, 2019 •

edited

Loading

evedon Sep 10, 2019 •

edited

Loading

hugueskamba Sep 10, 2019 •

edited

Loading

hugueskamba Sep 10, 2019 •

edited

Loading

hugueskamba commented Sep 11, 2019 •

edited

Loading